Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: deprecate two- and one-argument AtExit() #30227

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 2, 2019

Using AtExit() without an Environment* pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using AtExit() as an addon when the addon-targeting
AddEnvironmentCleanupHook() would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to AddEnvironmentCleanupHook() rather than AtExit().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 2, 2019
@addaleax addaleax added embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 2, 2019
@@ -3,7 +3,11 @@
#include <node.h>
#include <v8.h>

// TODO(addaleax): Maybe merge this file with the cctest for AtExit()?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to keep this as a todo or should we open an issue instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can open an issue about it once this lands. 👍

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 4, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit that referenced this pull request Nov 5, 2019
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member Author

addaleax commented Nov 5, 2019

Landed in 5b2067c, thanks for the reviews!

@addaleax addaleax closed this Nov 5, 2019
@addaleax addaleax deleted the atexit-deprecate branch November 5, 2019 23:03
addaleax added a commit to addaleax/node that referenced this pull request Nov 5, 2019
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: nodejs#30227 (comment)
@addaleax addaleax mentioned this pull request Nov 5, 2019
3 tasks
Trott pushed a commit that referenced this pull request Nov 8, 2019
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
Notable changes:

* addons:
  * Deprecate one- and two-argument `AtExit()`. Use the three-argument
    variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead
    (Anna Henningsen) nodejs#30227
* child_process,cluster:
  * The `serialization` option is added that allows child process
    IPC to use the V8 serialization API (to e.g., pass through data
    types like sets or maps) (Anna Henningsen)
    nodejs#30162
* deps:
  * Update V8 to 7.9
  * Update `npm` to 6.13.0 (Ruy Adorno)
    nodejs#30271
* embedder:
  * Exposes the ability to pass cli flags / options through an API
    as embedder (Shelley Vohr)
    nodejs#30466
  * Allow adding linked bindings to Environment (Anna Henningsen)
    nodejs#30274
* esm:
  * Unflag --experimental-modules (Guy Bedford)
    nodejs#29866
* stream:
  * Add `writable.writableCorked` property (Robert Nagy)
    nodejs#29012
* worker:
  * Allow specifying resource limits (Anna Henningsen)
    nodejs#26628
* v8:
  * The Serialization API is now stable (Anna Henningsen)
    nodejs#30234

PR-URL: nodejs#30547
MylesBorins added a commit that referenced this pull request Nov 21, 2019
Notable changes:

* addons:
  * Deprecate one- and two-argument `AtExit()`. Use the three-argument
    variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead
    (Anna Henningsen) #30227
* child_process,cluster:
  * The `serialization` option is added that allows child process
    IPC to use the V8 serialization API (to e.g., pass through data
    types like sets or maps) (Anna Henningsen)
    #30162
* deps:
  * Update V8 to 7.9
  * Update `npm` to 6.13.0 (Ruy Adorno)
    #30271
* embedder:
  * Exposes the ability to pass cli flags / options through an API
    as embedder (Shelley Vohr)
    #30466
  * Allow adding linked bindings to Environment (Anna Henningsen)
    #30274
* esm:
  * Unflag --experimental-modules (Guy Bedford)
    #29866
* stream:
  * Add `writable.writableCorked` property (Robert Nagy)
    #29012
* worker:
  * Allow specifying resource limits (Anna Henningsen)
    #26628
* v8:
  * The Serialization API is now stable (Anna Henningsen)
    #30234

PR-URL: #30547
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants